Skip to content

gcc: tighten condition for inhibit_libc=true#241206

Merged
Artturin merged 1 commit intomasterfrom
unknown repository
Oct 5, 2023
Merged

gcc: tighten condition for inhibit_libc=true#241206
Artturin merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Jul 3, 2023 •
edited by ghost
Loading

Description of changes

The situation described in the comment preceding export inhibit_libc=true does not match the conditional which follows it. Specifically, the comment says that this line is meant for "clang builds gcc" situations, yet it is enabled even when !stdenv.cc.isClang.

This commit tightens the conditional to make it match the situation described in the comment.

Needed for:

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ghost ghost requested a review from matthewbauer as a code owner July 3, 2023 02:44
@ghost ghost mentioned this pull request Jul 3, 2023
Closed
12 tasks
@ghost ghost requested a review from trofi July 3, 2023 03:09
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Jul 3, 2023
@trofi
Copy link
Contributor

trofi commented Jul 3, 2023

Why the change is needed? Does it fix anything?

If it does then I suggest update the the description to say so. And maybe comment should hint why it's undesirable for other cases.

gcc -> clang is only one example where config does not change. Another one would be pkgsCross.gnu64:

nix-repl> pkgsCross.gnu64.stdenv.hostPlatform == stdenv.hostPlatform
false

pkgsCross.gnu64 requires building a new libc without reliance on the host libc.

gcc: tighten condition for inhibit_libc=true
926c920 
The situation described in the comment preceding `export
inhibit_libc=true` does not match the conditional which follows it.
Specifically, the comment says that this line is meant for "clang
builds gcc" situations, yet it is enabled even when
`!stdenv.cc.isClang`.

This commit tightens the conditional to make it match the situation
described in the comment.
@ghost
Copy link
Author

ghost commented Jul 12, 2023

Why the change is needed?

Because what the comment says and what the code does are not the same. One of them must change. If you prefer to change the comment please submit a PR that explains why we inhibit_libc in such a huge class of situations.

If it does then I suggest update the the description to say so.

I think it's the other way around -- right now we have this inhibit_libc=true with no description of why it's there (except if stdenv.cc.isClang).

Does it fix anything?

Yes, right now there's no way to not turn on inhibit_libc for the first-stage cross compiler, like #241208 does.

inhibit_libc doesn't just prevent gcc from using the buildPlatform headers -- it prevents it from using any headers at all. So if we already have the targetPlatform headers, this inhibit_libc is blocking us from being able to use them.

@trofi
Copy link
Contributor

trofi commented Jul 12, 2023

I'll remove myself from reviewers then.

@trofi trofi removed their request for review July 12, 2023 21:22
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 20, 2023
@ghost
Copy link
Author

ghost commented Jul 20, 2023

Rebased.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 20, 2023
@Artturin
Copy link
Member

Artturin commented Oct 4, 2023 •
edited
Loading

Rebuilds caused by
nixpkgs_issue___screenshot_2023-10-04_22-53-28_123426745

The conditional was added in ea8e124 #182666

No diff in pkgsLLVM.hello

pkgsCross.aarch64-multiplatform.hello no longer has inhibit_libc=true

rnix-lsp
pkgsCross.aarch64-multiplatform.hello
pkgsCross.aarch64-multiplatform.pkgsLLVM.stdenv.cc

work

@Artturin Artturin merged commit c748544 into NixOS:master Oct 5, 2023
@yu-re-ka
Copy link
Contributor

yu-re-ka commented Oct 7, 2023

This broke pkgsMusl.nix:

       error: attribute 'useLLVM' missing

       at /nix/store/m2vqij78lfy9rvrmws264jpr7q78q98w-source/pkgs/development/compilers/gcc/common/pre-configure.nix:125:45:

          124|                       targetPlatform.config == hostPlatform.config &&
          125|                       (stdenv.cc.isClang || stdenv.targetPlatform.useLLVM)) ''
             |                                             ^
          126|   export inhibit_libc=true



$ nix eval -f . pkgsMusl.pkgsStatic.stdenv.targetPlatform.useLLVM
error: attribute 'useLLVM' in selection path 'pkgsMusl.pkgsStatic.stdenv.targetPlatform.useLLVM' not found

$

@Artturin
Copy link
Member

Artturin commented Oct 7, 2023

#259592

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Oct 9, 2023 •
edited
Loading

This also broke pkgsMusl.pkgsStatic.buildPackages.gccWithoutTargetLibc.cc, which is also required for pkgsMusl.nix: https://termbin.com/wf5i

@alyssais
Copy link
Member

alyssais commented Oct 10, 2023

#260181

@trofi trofi mentioned this pull request Oct 13, 2023
Merged
13 tasks
@ghost ghost deleted the tighten-inhibit_libc-condition branch January 23, 2024 06:52
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@trofi @Artturin @yu-re-ka @alyssais